feat(cashu): add_cashu_escrow_action lock handler — Track A TA-1#829
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughTrack A now implements Cashu escrow locking with validation, atomic persistence, token uniqueness, replay handling, notifications, database protections, and live-mint integration coverage. Dispatch expectations and Track A documentation are updated accordingly. ChangesCashu escrow lock
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Seller
participant Handler
participant Mint
participant Database
participant Buyer
Seller->>Handler: Submit CashuLockProof
Handler->>Database: Check token usage
Handler->>Mint: Verify token and lock conditions
Mint-->>Handler: Verification result
Handler->>Database: Atomically store escrow and set Active
Database-->>Handler: Commit or classified zero-row result
Handler->>Buyer: Enqueue CashuEscrowLocked
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2f032a652
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let locked = update_order_cashu_escrow( | ||
| pool, | ||
| order.id, | ||
| &configured_mint, | ||
| &proof.token, |
There was a problem hiding this comment.
Reject escrow proofs already locked to another order
This CAS is scoped only to the current order.id; when the same buyer/seller trade keys and amount are used on another order, the exact same unspent Cashu token passes the mint/key/amount checks again and can be stored on both orders. Because the mint only sees the proofs as unspent until one redemption, this can mark multiple trades Active while only one redeemable escrow exists; add a cross-order proof/Y uniqueness check before accepting the token.
Useful? React with 👍 / 👎.
e2f032a to
760467f
Compare
Fill in the CF-5 stub with the full escrow-lock algorithm
(docs/cashu/02-track-a-lock.md §4). A `cashu`-mode node now accepts a
seller's `AddCashuEscrow`, fully validates the 2-of-3 token against the mint
and the order's trade keys, atomically advances `WaitingPayment → Active`,
publishes the updated order event, and notifies the buyer to send fiat.
Validate-fully-then-commit discipline (same as `release_action`):
1. resolve the order;
2. authorise the sender == the order's seller trade key (else `InvalidPeer`);
3. require `WaitingPayment` status;
4. extract the `CashuLockProof` (absent ⇒ `InvalidCashuToken`);
5. bind the mint: `proof.mint_url` must equal the configured mint
(`InvalidMintUrl`) — a cheap pre-check; step 7 enforces the authoritative
token↔mint binding;
6. bind `{P_B, P_S, P_M}` to THIS order — reject a proof whose stated keys
disagree with the order, and derive the keys handed to the mint from the
order (never from the proof), so the 2-of-3 can only lock to keys Mostro
already holds;
7. `verify_escrow_token` (2-of-3 + seller-recovery locktime floor
`now + escrow_locktime_days` + mint + amount + DLEQ + unspent); mint
unreachable ⇒ `CashuMintUnavailable`, malformed ⇒ `InvalidCashuToken`;
8. `update_order_cashu_escrow` CAS (lock + status advance in one write);
zero rows ⇒ idempotent no-op (replay/concurrent), no notification;
9. publish the Active order event (best-effort, logged on failure);
10. notify buyer + seller with `CashuEscrowLocked`.
The escrow token locks `order.amount` exactly (Option 2 — the Mostro fee is a
separate token added in TA-1f).
Tests: the deterministic pre-mint rejection paths (wrong sender ⇒
`InvalidPeer`, wrong status, missing proof ⇒ `InvalidCashuToken`) and the
`cashu_reason` error mapping. The mint-backed happy-path + replay tests are a
follow-up: they need a 2-of-3 token builder in the CF-3 harness (which today
ships only connectivity helpers).
Depends on CF-5 (dispatch seam + cashu_client). Base: feat/cashu-cf5-boot-integration
Refs: docs/cashu/02-track-a-lock.md (TA-1)
TA-1 implements AddCashuEscrow, so dispatch_cashu no longer routes it to InvalidAction — it runs the real lock handler. Remove it from the 'blocks every order-lifecycle action' assertion list.
… guard Two findings from the automated review of #829. Replay recovery (step 3b). The step-10 notifications are sent after the CAS commits, so a crash or a lost send queue in between left the escrow funded and the buyer never cued to send fiat — and the step-3 status check rejected every retry, making the documented idempotent retry path unreachable. An order this handler already locked (escrow stored, still Active) whose submitted token matches the stored one now replays the notifications and returns Ok(()): no mint round-trip, no second write. A different token on a locked order is a second funding the escrow can never honour, so it is rejected. The exception is scoped to Active, so a stale "escrow locked, send fiat" is never replayed onto a trade that has moved on. Cross-order token uniqueness (step 6b). The 2-of-3 condition commits to trade keys, not to an order id, and the mint reports proofs unspent until the first redeem — so two orders reusing the same trade keys could both validate the very same token and go Active against one redeemable escrow. The handler now rejects a token already escrowed elsewhere before the mint round-trip, and the CF-4 CAS carries a NOT EXISTS leg that closes the check-then-act race atomically. A CAS that matches zero rows because another order took the token is reported as InvalidCashuToken rather than a silent no-op. This is token-level uniqueness; proof-level (Y = hash_to_curve) uniqueness arrives with TA-1f. Track A §4 of the spec is updated to match, along with its Definition of Done.
760467f to
18cc7d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/db.rs (1)
826-861: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd an index for the Cashu escrow token uniqueness predicate.
cashu_escrow_fieldsonly adds the columns, whileupdate_order_cashu_escrowandcashu_escrow_token_in_useboth filterordersbycashu_escrow_token; add a migration index for that column to avoid fullorderstable scans on lock/check attempts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db.rs` around lines 826 - 861, Add a database migration creating an index on orders.cashu_escrow_token, alongside the schema change represented by cashu_escrow_fields. Ensure the index supports the token lookups in update_order_cashu_escrow and cashu_escrow_token_in_use without changing their query behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/add_cashu_escrow.rs`:
- Around line 244-263: Update the !locked CAS-failure handling in the escrow
lock flow to first verify that order.id is locked with the submitted proof.token
before treating the result as an idempotent no-op. If the order is locked with a
different token, reject with InvalidCashuToken; retain the existing rejection
for the token being locked to another order and the no-op behavior only when the
stored token matches or the status has moved on.
---
Nitpick comments:
In `@src/db.rs`:
- Around line 826-861: Add a database migration creating an index on
orders.cashu_escrow_token, alongside the schema change represented by
cashu_escrow_fields. Ensure the index supports the token lookups in
update_order_cashu_escrow and cashu_escrow_token_in_use without changing their
query behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 798b24f0-9e03-42c6-bb24-231dfe3f34da
📒 Files selected for processing (4)
docs/cashu/02-track-a-lock.mdsrc/app.rssrc/app/add_cashu_escrow.rssrc/db.rs
Two follow-up review findings on the TA-1 lock handler. A compare-and-set that matched zero rows was treated as a no-op whenever the token was not held by another order. That misses one case: a concurrent submission may have locked THIS order with a *different* token in the window since the order was fetched, which means this submission was never accepted — answering Ok(()) leaves the seller believing an untouched, still-unspent token is escrowed. The classification now lives in `classify_zero_row_cas`, which rejects both lost races (token taken by another order; this order locked with another token) and keeps the no-op for a duplicate delivery of the winning token or a status that simply moved on. Extracting it also makes the branch unit-testable — the call site sits past the mint round-trip and cannot be reached from tests. The handler's two token lookups — the CAS's NOT EXISTS leg and cashu_escrow_token_in_use — were full table scans of `orders`. Adds a partial index on cashu_escrow_token, unique for the same reason as idx_bonds_parent_child_unique: it cannot change current behaviour (the CAS never attempts a duplicate write) but makes the one-order-per-token invariant survive a regression that drops the NOT EXISTS predicate. Track A §4 step 8 is updated to match.
The unit tests stop before the mint round-trip, so steps 5-8 of the handler — mint binding, pubkey binding, verify_escrow_token, the CAS — had no coverage at all. tests/cashu_mint.rs cannot fill the gap either: mostro is a binary crate with no lib target, so an integration test cannot reach the handler. Adds an in-tree #[ignore]d, env-gated harness that mints a real 2-of-3 escrow token (data = P_S, pubkeys = [P_B, P_M], n_sigs = 2, locktime, refund = [P_S]) at a live mint and drives add_cashu_escrow_action with it end to end: happy path WaitingPayment -> Active, then the two guards this PR added — replay recovery and cross-order token reuse. It mints straight into the P2PK condition rather than minting plain ecash and swapping, so the token value stays exactly the order amount on a mint that charges an input fee (mint.cubabitcoin.org's sat keyset charges 100 ppk, and a swap would eat into the amount the handler checks for equality). Safety, since it can run against a real mint: the three keys are generated per run and written to disk BEFORE anything is minted — they are the only way to redeem the escrow — and the locktime floor defaults to 1 day rather than the 15-day production default, so the seller-recovery path opens quickly. Verified against the throwaway nutshell mint: all three checks pass. A plain cargo test still reports it as ignored.
What & why
The first functional Cashu flow (
docs/cashu/02-track-a-lock.md, TA-1): fills in the CF-5 stubadd_cashu_escrow_actionwith the full escrow-lock algorithm. Acashu-mode node now accepts a seller'sAddCashuEscrow, fully validates the 2-of-3 token against the mint and the order's trade keys, atomically advancesWaitingPayment → Active, publishes the updated order event, and notifies the buyer to send fiat.Validate-fully-then-commit discipline (same as
release_action): compute/verify first, persist second, notify last. A validation or persistence failure leaves the order exactly as it was.Algorithm (§4)
InvalidPeerWaitingPaymentNotAllowedByStatusCashuLockProofInvalidCashuTokenproof.mint_url== configured mint (cheap pre-check)InvalidMintUrl{P_B, P_S, P_M}to this order (reject mismatched proof keys; derive the mint-validation keys from the order, never the proof)InvalidCashuTokenverify_escrow_token(2-of-3 + seller-recovery locktime floor + mint + amount + DLEQ + unspent)InvalidCashuToken/CashuMintUnavailableupdate_order_cashu_escrowCAS (lock + advance in one write); zero rows ⇒ idempotent no-opCashuEscrowLockedThe escrow token locks
order.amountexactly (Option 2 — the Mostro fee is a separate token added in TA-1f).Security core (step 6)
The 2-of-3 must lock to the keys Mostro already holds for the order, never attacker-chosen keys. The handler both rejects a proof whose stated
buyer/seller/mostro_pubkeydisagree with the order, and derives the{P_B, P_S, P_M}handed toverify_escrow_tokenfrom the order (the seller isevent.sender, already authorised in step 2) — so the proof's own key fields can never widen the accepted set.Tests
InvalidPeer; wrong status; missing proof ⇒InvalidCashuToken(order untouched).cashu_reasonerror mapping (mint-unreachable ⇒ retryableCashuMintUnavailable; bad token ⇒InvalidCashuToken; bad URL ⇒InvalidMintUrl).dispatch_cashugate test is updated:AddCashuEscrowis no longer in theInvalidActionlist (it runs the real handler now).Follow-up — mint-backed integration test
The happy-path (valid lock →
Active+ buyer notified) and the replay no-op require constructing a real 2-of-3 P2PK token (locktime +refund=[P_S]+ DLEQ) via a wallet mint+swap against the CF-3 nutshell mint. The CF-3 harness (tests/common/mod.rs) currently ships only connectivity helpers (mint_url_from_env,wait_for_mint,mint_get_json) — no 2-of-3 token builder yet. That builder + the env-gated#[ignore]happy-path/replay tests are a focused follow-up, not folded in here to keep the PR reviewable.Checklist
cargo fmt --checkcargo clippy --all-targets --all-features -- -D warningscargo test— 1026 passedDepends on #828 (CF-5). Refs:
docs/cashu/02-track-a-lock.md(TA-1).🧪 Manual testing — step by step
Prereqs: the same Cashu-mode
mostrodsetup as CF-5 (#828) — the throwaway mint up (docker compose -f docker-compose.cashu.yml up -d,http://127.0.0.1:3338) and[cashu] enabled = true. This branch is stacked on CF-5, so check it out (it contains CF-5 + TA-1). Run withRUST_LOG=mostro=info.1. Deterministic rejection paths (no token needed)
Seed a
WaitingPaymentsell order for a known seller trade key (create+take on the TA-2 stack, or insert a row directly). Then submitAddCashuEscrowand confirm theCantDoReason:CantDo(InvalidPeer). ✔️WaitingPayment→CantDo(NotAllowedByStatus). ✔️AddCashuEscrowwith a non-CashuLockProofpayload →CantDo(InvalidCashuToken). ✔️mint_urldiffers from the node's configured mint →CantDo(InvalidMintUrl). ✔️The order is left unchanged after every rejection (status still
WaitingPayment,cashu_escrow_tokenstill NULL). ✔️2. Unit + mint-backed library checks
cargo test --bin mostrod add_cashu_escrowcashu_reasonmapping tests pass. ✔️CashuClient::verify_escrow_tokenchecks against the live mint:CASHU_TEST_MINT_URL=http://127.0.0.1:3338 cargo test cashu -- --ignored --nocapture3. Happy-path lock (full stack — TA-2 + token-building client)
WaitingPayment, then have the seller's client build the 2-of-3 token (locktime +refund=[P_S], DLEQ) and submitAddCashuEscrow.WaitingPayment → Activein one atomic write, the updated order event is published, and both parties receiveCashuEscrowLocked(the buyer's cue to send fiat). A replayed submission is a safe no-op (matches zero rows). ✔️Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests